Fix duplication of Randomized Content Blocks - #7482

Thanks for the pull request, @Kelketek! It looks like you're a member of a company that does contract work for edX. If you're doing this work as part of a paid contract with edX, you should talk to edX about who will review this pull request. If this work is not part of a paid contract with edX, then you should ensure that there is an OSPR issue to track this work in JIRA, so that we don't lose track of your pull request.

To automatically create an OSPR issue for this pull request, just visit this link: http://openedx-webhooks.herokuapp.com/github/process_pr?repo=edx%2Fedx-platform&number=7482

@Kelketek 👍

@marcotuts Care to take a look at this? @antoviaque Anyone else I should ping here?

@Kelketek Yes, we need a second +1 -- we could ask someone else from solutions to look at this, but @bradenmacdonald since you investigated this originally and contributed to figuring out the fix, it might make sense for you to be the second reviewer - either now if you have the time, or on Monday as part of the next sprint.

@antoviaque Having both @e-kolpakov and @bradenmacdonald review this would be nice, but since this is for the upstream platform, we'll need to make sure someone from upstream gives it a +1 as well. I have a feeling you're factoring in @marcotuts already for that, but I just want to be clear in case. :)

(no comments from me on this and I generally won't review / thumbs up python fixes myself. That being said, I think it's ok if the second review is another member of this team. @antoviaque can make the call there.)

@Kelketek I meant to have Braden as the second reviewer, it was mainly library-specific changes so it would have made sense - but on a second look, the change to _duplicate_item() could likely use eyes from platform. @cpennington would you be able to review this PR?

Tested again. 👍 from me if you fix that .for_version issue I just flagged and test it. Would be good to add a test for that as well - sync the RCB, add another block to the library (triggering a new library version), then duplicate the RCB without syncing it.

Thanks for the pull request, @Kelketek! It looks like you're a member of a company that does contract work for edX. If you're doing this work as part of a paid contract with edX, you should talk to edX about who will review this pull request. If this work is not part of a paid contract with edX, then you should ensure that there is an OSPR issue to track this work in JIRA, so that we don't lose track of your pull request.

To automatically create an OSPR issue for this pull request, just visit this link: http://openedx-webhooks.herokuapp.com/github/process_pr?repo=edx%2Fedx-platform&number=7482

@Kelketek Oh, I was so close to giving you a thumbs up...

But then I found a bug. Here's a test case for it: a2cc1d5a1c58ee444a9f13251374e4f625a5ade0

I tried looking at the cause and have half of a fix for your consideration here: 433c7d9ad4e5bd300ea89c253ec0f92cd638ad09 (open-craft/fix_duplicate_rcb_review) - you need to change it so that copy_from_template accepts head_validation as an argument and passes it into self._lookup_course(). But it possibly has some side effects because I see a few other tests in that file are now giving errors.

@bradenmacdonald Thanks a lot for the careful review. Once the issue you've found is fixed, we'll technically have 2 +1 on the PR, but given the scope I originally thought of also involving platform in the review. Do you think that's still necessary? How confident are you and @Kelketek about the fix?

@cpennington Would you want to review this? Not sure if you had seen my ping on this PR earlier this week.

I'm asking because we are getting close to the next release, so if we need a review from platform it's starting to be very tight - we would basically need a review today. :/

Thanks, @cpennington . I'm testing the changes I made for Braden's notes now, and will work yours on top of those. I hope to have new changes pushed within an hour or so.

@cpennington I have chosen to address only your non-optional suggestions in the interest of time. Please take a look at the current changes, and if they're ok and tests pass, I will rebase to squash the commits and merge.

Ah, and if @bradenmacdonald gives his +1 as well.

@Kelketek I'd like to at least track the option of providing a default studio_post_duplicate. As I think about that option more, I'm more convinced that it's a better way to go than the code you have in the PR now.

@cpennington Alright, I'll go and hit it now. In the meantime, can you confirm the other issues are fine? I'll ping you once more as I get that change pushed. May as well do it now.

@cpennington The function is recursive normally, and that function is a private function of that module. Are you sure?

If so, because of those conditions, I am going to have to track it after all instead of fixing it now. But I did find one of my changes didn't make it into the commit, so I have just pushed that as well. I'll make that item in the tracker if you like.

Yes, I think moving it out as a private function is still clearer than having it nested (even with the recursion).

I'm 👍, as long as making studio_post_duplicate gets recorded as a cleanup to happen post-deadline.

@cpennington I've created a task in platform for this. Not sure I made the ticket quite right (or if that's the best place for it), but I've pinged you on it.

👍

Thanks @bradenmacdonald and @cpennington !